Skip to content

Fix unavailable sandbox toggle state#825

Merged
shanselman merged 1 commit into
mainfrom
shanselman/fix-sandbox-toggle-unavailable
Jun 26, 2026
Merged

Fix unavailable sandbox toggle state#825
shanselman merged 1 commit into
mainfrom
shanselman/fix-sandbox-toggle-unavailable

Conversation

@shanselman

@shanselman shanselman commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Port the sandbox unavailable-toggle fix onto current main without reverting newer MXC strict host-fallback and shell-approval behavior.
  • Normalize the Sandbox page toggle off when MXC is definitively unavailable, so the UI does not claim containment is on when it cannot run.
  • Reject turning Node Sandbox back on in the non-strict host-fallback mode while MXC is definitively unavailable; preserve strict fallback blocking so users who opted into command denial stay protected.
  • Add source-contract coverage for the normalization/rejection behavior and strict-blocking guard.

Validation

  • ./build.ps1
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore

Review notes

  • Original branch targeted stale master; this PR branch was replaced with a clean main port after maintainer approval.
  • Focused code review and security review both found a strict-blocking bypass in the first port; the final patch preserves strict blocking and both reviewers came back clean.

@clawsweeper

clawsweeper Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 25, 2026, 11:38 PM ET / 03:38 UTC.

Summary
This PR changes the WinUI Sandbox page to turn the toggle off and reject re-enabling it when MXC is definitively unavailable, plus source-contract tests for that behavior.

Reproducibility: yes. by source inspection: the PR head calls normalization from page refresh/load, sets SystemRunSandboxEnabled=false, then saves. I did not run the WinUI scenario because this review is read-only.

Review metrics: 2 noteworthy metrics.

  • Changed Surface: 2 files, +146/-9. The scope is narrow enough for focused review, but one runtime file controls sandbox settings behavior.
  • Automatic Save Paths: 1 added. The added normalization path writes a persisted sandbox setting without an explicit user opt-out.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Preserve the stored sandbox preference while rendering unavailable MXC as ineffective/off in the UI.
  • Update the contract tests so unsupported-host page visits and presets do not permanently flip SystemRunSandboxEnabled without explicit user opt-out.
  • [P1] Add redacted unavailable-MXC proof after the code behavior is corrected.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists validation commands but no redacted screenshot, terminal output, logs, recording, or artifact showing the unavailable-MXC UI behavior after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A real Windows settings-page capture on an MXC-unavailable host would materially prove the visible toggle/dialog behavior after the code blocker is fixed. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify the Sandbox page on an MXC-unavailable Windows host shows the final unavailable toggle/dialog behavior with redacted diagnostics.

Risk before merge

  • [P1] Opening the Sandbox settings page on an unsupported-MXC host can save SystemRunSandboxEnabled=false for users who previously left sandboxing on, so future MXC availability would not restore contained execution.
  • [P1] The PR lacks redacted real behavior proof from an unavailable-MXC Windows setup; listed build/test commands are supplemental for an external contributor PR.

Maintainer options:

  1. Preserve The Preference Before Merge (recommended)
    Render unavailable MXC as ineffective/off in the page without saving SystemRunSandboxEnabled=false, and update contract tests to protect upgrade behavior.
  2. Accept Persist-Off As A Product Choice
    Maintainers can intentionally accept the persisted opt-out behavior, but should add explicit upgrade and future-MXC proof and own the security tradeoff.
  3. Pause Until Real Proof Exists
    Keep the PR unmerged until redacted unavailable-MXC UI proof shows the final toggle and rejection behavior after any code repair.

Next step before merge

  • [P1] Needs contributor real behavior proof plus maintainer handling of the persisted sandbox preference regression; automation cannot supply the contributor's unavailable-MXC setup proof.

Security
Needs attention: The diff can weaken the sandbox boundary by persisting an implicit sandbox opt-out from an unavailable-MXC UI refresh.

Review findings

  • [P1] Preserve the stored sandbox preference — src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs:365-373
Review details

Best possible solution:

Render unavailable MXC as an effective off/unavailable UI state without mutating the stored sandbox preference; preserve strict fallback blocking and require redacted unavailable-host proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes, by source inspection: the PR head calls normalization from page refresh/load, sets SystemRunSandboxEnabled=false, then saves. I did not run the WinUI scenario because this review is read-only.

Is this the best way to solve the issue?

No, not as submitted. The safer fix is to separate the effective unavailable UI state from the persisted user preference unless maintainers explicitly approve and prove a persist-off upgrade behavior.

Full review comments:

  • [P1] Preserve the stored sandbox preference — src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs:365-373
    When MXC is definitively unavailable, this path writes SystemRunSandboxEnabled=false and saves it during page refresh. Current main treats sandbox-on plus non-strict unavailable MXC as a compatibility fallback state, so opening this page can silently erase the user's desired sandbox-on preference and leave future commands uncontained after MXC becomes available again.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5602c96c2704.

Label changes

Label justifications:

  • P1: The PR touches sandbox settings behavior and can silently disable future contained execution for existing unsupported-MXC users.
  • merge-risk: 🚨 compatibility: The diff writes an existing persisted sandbox preference to false during page refresh on unsupported hosts.
  • merge-risk: 🚨 security-boundary: The setting controls whether future system.run commands use MXC containment once available, and rewriting it can leave commands uncontained.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists validation commands but no redacted screenshot, terminal output, logs, recording, or artifact showing the unavailable-MXC UI behavior after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

Security concerns:

  • [medium] Automatic opt-out can disable future containment — src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs:365
    Saving SystemRunSandboxEnabled=false on an unsupported host means the user's sandbox-on preference will not automatically resume containment after MXC is installed or repaired.
    Confidence: 0.88

What I checked:

Likely related people:

  • TheAngryPit: Commit ce3294d added the current strict fallback setting, unavailable-MXC behavior, and related tests that this PR must preserve. (role: introduced fallback contract; confidence: high; commits: ce3294d40624; files: src/OpenClaw.Shared/Mxc/MxcCommandRunner.cs, src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs, tests/OpenClaw.Shared.Tests/Mxc/MxcCommandRunnerTests.cs)
  • bkudiess: Commit 9f4d238 carried recent MXC probe and Sandbox page work on the same settings surface. (role: recent MXC/Sandbox page contributor; confidence: high; commits: 9f4d23804122; files: src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs, src/OpenClaw.Shared/Mxc/MxcAvailability.cs, src/OpenClaw.Shared/Mxc/MxcCommandRunner.cs)
  • RBrid: Commit 1a07759 recently touched sandbox unavailable messaging and notification behavior adjacent to this UI path. (role: recent adjacent contributor; confidence: medium; commits: 1a07759a7e6e; files: src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs, src/OpenClaw.Tray.WinUI/App.xaml.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@shanselman

shanselman commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Quarantining this for now because it targets master, while the repository default and landing branch is main.

The PR branch looks small as a single commit, but comparing it against current main pulls in the old master history and produces a polluted diff, so we should not review or merge it in this state. Please retarget/rebase this branch onto main; then we can do a real review and validation pass.

Also flagging that this touches sandbox unavailable / host fallback behavior, which has changed recently on main, so it needs review against the current strict fallback setting semantics before landing.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 25, 2026
@shanselman shanselman changed the base branch from master to main June 26, 2026 02:24
@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 26, 2026
@shanselman
Fix unavailable sandbox toggle state
9147d1d 
Normalize the Sandbox page toggle off when MXC is definitively unavailable, reject turning it back on in the host-fallback mode, and preserve strict fallback blocking so users who opted into command denial remain protected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@shanselman shanselman force-pushed the shanselman/fix-sandbox-toggle-unavailable branch from 124fdfa to 9147d1d Compare June 26, 2026 03:29
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Jun 26, 2026
@shanselman shanselman merged commit cd02def into main Jun 26, 2026
20 checks passed
@shanselman shanselman deleted the shanselman/fix-sandbox-toggle-unavailable branch June 26, 2026 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@shanselman